-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Use test name for dir when running tests #11738
Conversation
r? @epage (rustbot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. The tests output looks pretty good!
Could you update relevant doc here as wellhttps://doc.crates.io/contrib/tests/writing.html? Thanks! |
Cleanup tests When working on #11738 I noticed a few tests that needed to be cleaned up. It [was suggested](#11738 (comment)) to split the test changes into their own PR. - `bad_config::bad1` - This was trying to match `config` which would get matched in the wrong place due to the test name having `config` in it - Fixed by making the match `/config` - `build_script::panic_abort_with_build_scripts` - This test was failing as it was making sure `panic` was not in the output. After this change, it could match the test name and fail. - To fix this I updated it to look at `panic=abort` which appears to be what it was originally looking for (#5711). `@ehuss` please let me know if I misread what the test was testing. - `cargo_command::cargo_subcommand_args` - This test had `#[test]` above `#[cargo_test]` which caused it to throw an error removing it fixed the issue During this time I also ran into issues with `cargo_remove` tests being named `fn case()` which is different from how tests are normally named. I talked with `@epage` and it was decided that `fn case()` should probably be used for all `snapbox` tests since the unique test name is already the folder name. I went ahead and renamed all tests within `cargo_add/` and `init/` to match this style. This PR should be reviewed commit by commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me btw. Thank you for putting efforts on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice! Thank you.
@bors r+ |
feat: Use test name for dir when running tests Tests for Cargo currently put their generated data in `target/tmp/cit/t{}`. This can make it very hard to debug tests if multiple are falling. Having the tests be named would make this much easier when debugging. [This comment](https://github.com/rust-lang/cargo/blob/eff8d693b99077b0b7f70e4bc414fdca53597085/crates/cargo-test-support/src/paths.rs#L54-L64) explains why cargo used a number instead of the test name for the unique identifier. I found a way to get the test name to be the unique identifier. I did this by having `cargo-test-macro` get the name of the test when the macro is looking for where the function body starts. It passes the name of the test and the file that the test comes from into the boilerplate it was already adding. It uses the file and the test name as the directory to place the tests generated data data into, i.e, `target/tmp/cit/testsuite/workspaces/workspace_in_git`. ![Screenshot 2023-02-20 at 20 55 30](https://user-images.githubusercontent.com/23045215/220236036-e1dcbe53-033e-4db7-a6a3-a689eae97386.png) Note: I also found `t{}` to be frustrating when trying to get the size of a test. There is no good way to get the size of a test within `target/tmp/cit`, without running the tests one at a time and checking the size like so: ```python for test in tests: subprocess.run(["cargo", "test", "-p", "cargo", test, "--", "--exact"]) test_size = subprocess.run(["du", "-sk", "target/tmp/cit/t0"], stdout=subprocess.PIPE, text=True) size_dict[test] = int(test_size.stdout.split()[0]) ``` It made it very hard to try and find which tests were causing most of the size of the `target` dir.
💔 Test failed - checks-actions |
I'd like to mention that this will make development more difficult for my workflow. I usually leave a terminal in the I understand there are benefits here, but I wanted to highlight that it will cause some difficulties. |
☀️ Test successful - checks-actions |
I think this is a case where we can't satisfy everyone's workflow. I've found the current approach is hard to work as go to inspect a specific failure from a test run, I can't predict what directory will be used. Now, I can always predict it. This also opens us up to test parallelism (#5609) once its implemented and to |
23 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..c1334b059c6dcceab3c10c81413f79bb832c8d9d 2023-02-28 19:39:39 +0000 to 2023-03-07 19:21:50 +0000 - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) Note that some 3rd-party licensing allowed list changed due to the introducion of `gix` dependency
Revert "#11738" - Use test name for dir when running tests
2 commits in 1334b059c6dcceab3c10c81413f79bb832c8d9d..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-03-07 19:21:50 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810)
Update cargo 25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810) - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) --- ~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess? r? `@ehuss` cc `@Muscraft`
Update cargo 25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810) - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) --- ~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess? r? `@ehuss` cc `@Muscraft`
Tests for Cargo currently put their generated data in
target/tmp/cit/t{}
. This can make it very hard to debug tests if multiple are falling. Having the tests be named would make this much easier when debugging.This comment explains why cargo used a number instead of the test name for the unique identifier. I found a way to get the test name to be the unique identifier. I did this by having
cargo-test-macro
get the name of the test when the macro is looking for where the function body starts. It passes the name of the test and the file that the test comes from into the boilerplate it was already adding.It uses the file and the test name as the directory to place the tests generated data data into, i.e,
target/tmp/cit/testsuite/workspaces/workspace_in_git
.Note: I also found
t{}
to be frustrating when trying to get the size of a test. There is no good way to get the size of a test withintarget/tmp/cit
, without running the tests one at a time and checking the size like so:It made it very hard to try and find which tests were causing most of the size of the
target
dir.